Review period will end on 2021-01-18 at 21:05:31 UTC.

LGTM. Maybe someday we'll move each named_args line to just before or after the description, but that's a discussion for another time.

I don't think I would be opposed to this. I'd need to see how it looks I guess.

Separately, I'm a little confused about the difference between flag "--foo=" and flag "--foo" and I'm wondering if one of you can clarify.

Based on experimentation, it seems that flag "--foo=" means that one doesn't need to use = when passing something to the flag. flag "--foo" does require =.

# flag "--foo="
brew cmd --foo=bar # works
brew cmd --foo bar # works

# flag "--foo"
brew cmd --foo=bar # works
brew cmd --foo bar # doesn't work (treats `bar` as normal named argument)

It seems that adding = passes OptionParser::REQUIRED_ARGUMENT to @parser.on rather than OptionParser::OPTIONAL_ARGUMENT. The wording here is throwing me off a little bit. Whether or not = is used doesn't seem to determine whether or not an argument is is required. Am I missing something here?

Oh wait, I'm wrong. Here's an updated explanation:

# flag "--foo="
brew cmd --foo=bar # args.foo returns "bar"
brew cmd --foo bar # args.foo returns "bar"
brew cmd --foo     # args.foo throws an error: "missing argument: --foo"

# flag "--foo"
brew cmd --foo=bar # args.foo returns "bar"
brew cmd --foo bar # args.foo returns true
brew cmd --foo     # args.foo returns true

So the question becomes: how should flags in the usage string show up? It seems that [--foo=foo] is the safe option. For flags that do require an argument (i.e. with =), should we use [--foo foo] instead? Something else?

In the cases where the argument isn't required, should there be a way to indicate that [--foo] or [--foo=bar] are allowed? Using [--foo[=foo]] or [(--foo|--foo=foo)] feels a little too complex.

Maybe someday we'll move each named_args line to just before or after the description, but that's a discussion for another time.

@EricFromCanada I have to say I'm torn about this. I do kind of like them at the end (although I'm not totally sure why). It also adds some consistency with the usage string (in which named args come after options). On the other hand, it's nice to be able to see the named_args at a glance toward the top of the command_args method. I guess I'm leaning toward no change in this PR, but I'd be open to changing it in the future.

Just so we're clear, the = at the end of a flag declaration indicates that supplying a value is required, and the lack of one indicates a default value will be passed. For example, brew info --json --all is equivalent to brew info --json=v2 --all.

I'd try to keep complexity down in the usage string, so I'd be fine with just [--foo] for flags with default values and [--foo=] for flags that require a value.

Just so we're clear, the = at the end of a flag declaration indicates that supplying a value is required, and the lack of one indicates a default value will be passed. For example, brew info --json --all is equivalent to brew info --json=v2 --all.

The default value isn't actually passed, though, it just needs to be inferred by the code that handles the option.

I'd try to keep complexity down in the usage string, so I'd be fine with just [--foo] for flags with default values and [--foo=] for flags that require a value.

Okay, let's go with this. I like it: not too cluttered and still easy to understand.

Review period ended.